- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 420
Revised build importer implementations #5428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| No reason I'm aware of, we can make them optional | 
| I think the Maven Central validator rejects projects without these fields. Although it's specific to Maven Central, having these fields mandatory avoids some late pitfalls, when publishing artifacts. We only use  | 
| We could allow  | 
1d3b959    to
    3ba698a      
    Compare
  
    3088228    to
    e398858      
    Compare
  
    | A new SBT importer implementation has been added that can import projects that use  This was tested against 12 projects and all but 2 were successfully imported. 
 One of the tests can be run using the following command: ./mill standalone.migrate[sbt].testOnly mill.standalone.MillInitAirstreamTests | 
| I am going to be working on adding support for some more settings such as  What are your thoughts on supporting more plugins? We already have tests that require the following: 
 | 
| @ajaychandran update the PR title and description to summarize the motivation, major changes, how it integrates with the existing codebase, and testing strategy. It's a large PR and that would make it easier to review | 
| ps.println() | ||
| ps.print("def repositories = super.repositories() ++ ") | ||
| ps.print("Seq(") | ||
| ps.print(literalize(repositories.head)) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use string interpolation for this and all similar code snippets so it's not so verbose
| import mill.constants.OutFiles.{millDaemon, millNoDaemon, out} | ||
| import os.{Path, ProcessInput, ProcessOutput, Shellable} | ||
|  | ||
| class StandaloneTester( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be different from normal IntegrationTester? We should make these normal integration tests using the normal IntegrationTester utilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't have the same requirements as IntegrationTester. The workspace creation and initialization happens outside the tester.
Given that this PR proposal will take some time, I chose to isolate this to avoid any conflicts when re-basing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What workspace creation and initialization do we expect with IntegrationTester? If run on an empty folder, the workspace starts empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IntegrationTester clears the workspace and copies resources. I suppose this can be circumvented with an empty resource folder.
I am not opposed to the idea of sharing existing code. I just kept all new code isolated initially. I will merge the testkit classes and move the tests to integration/manual.
        
          
                standalone/package.mill
              
                Outdated
          
        
      | @@ -0,0 +1,33 @@ | |||
| package build.standalone | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put this in the integration/manual/, indicating that these are tests meant to be run manually but won't be run in CI, but are otherwise normal integration tests
| @@ -0,0 +1,108 @@ | |||
| package mill.init.migrate | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this file related to the code we already have in libs/init/buildgen/src/mill/main/buildgen/ir.scala? Does it replace it entirely, or does it re-use parts of the existing model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is meant to replace it.
In essence, it is a refactoring of the existing model that organizes settings into groups that correspond to actual module types.
Also, the IR types with the rendered code are not required and do not exist in the new model. This was primarily introduced, by me, for file merging. The new implementation achieves the same in a sane way.
| "org.testng" -> "TestModule.TestNg", | ||
| "org.scalatest" -> "TestModule.ScalaTest", | ||
| "org.specs2" -> "TestModule.Specs2", | ||
| "com.lihaoyi" -> "TestModule.Utest", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit imprecise. For example a lot of people use com.lihaoyi::os-lib but don't use utest. Could we match on a whitelist of artifact names (or prefixes, ignoring the _{scala-version}) as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this seems to duplicate logic we already have in BuildgenUtil. We should consolidate it and avoid copy-pasting code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is the same as the list in BuildGenUtil.
Given that this is a rewrite of the core design, I have kept the new implementation separate. If the proposal is approved, this can be consolidated.
|  | ||
| import upickle.default.{ReadWriter, macroRW} | ||
|  | ||
| case class Tree[+A](root: A, children: Seq[Tree[A]] = Nil) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this different from libs/init/buildgen/src/mill/main/buildgen/Tree.scala?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in Scala 2 and the unused features are removed.
But basically it is the same and only exists because the new implementation is isolated.
| } | ||
|  | ||
| def renderSbtPlatformModule = | ||
| s"""trait SbtPlatformModule extends SbtModule { outer => | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this different from SbtModule with PlatformScalaModule? If it's necessary, we should put it (and CrossSbtPlatformModule) in libs/scalalib/src/mill/scalalib/ rather than codegen-ing it every import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SbtModule resets sources
  override def sources = Task.Sources("src/main/scala", "src/main/java")and PlatformModuleBase operates on sourceFolders.
  override def sourcesFolders: Seq[os.SubPath] = super.sourcesFolders.flatMap {
    source => Seq(source, source / os.up / s"${source.last}-${platformCrossSuffix}")
  }Are these compatible in any scenario?
This is being used to replicate the layout used by sbt-crossproject plugin. But now we also have sbt-projectmatrix for cross development which seems to use a completely different structure. Is it okay to add a trait to scalalib per plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make SbtModule use sourceFolders as well, just for consistency with the rest.
I think having traits in scalalib for each of them is fine as long as they're meaningful and well documented .
| @@ -0,0 +1,225 @@ | |||
| package mill.init.migrate.sbt | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this relate to the existing libs/init/sbt/exportplugin/src/mill/main/sbt/ExportBuildPlugin.scala? Can we consolidate them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally they are the same.
But the script offers certain advantages such as not requiring any extra build files to add a plugin. The idea was borrowed from IDEA :)
| import scala.collection.mutable | ||
| import scala.util.Using | ||
|  | ||
| opaque type PackageTree = Tree[PackageRepr] | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just use a normal case class here with normal methods, rather than this opaque type with extensions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A PackageTree is supposed to have the following invariants:
- The root package is located at the workspace root.
- The tree structure represents the filesystem structure.
 Anopaque typeseemed the "right" choice.
 The extension methods can be defined as normal methods.
Could you provide the reason behind the comment? This is just for my learning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, we aim to use simpler language features over more powerful ones, unless there is a strong reason otherwise. In this case, there is no strong reason to use an opaque type, so we use case class which people are more familiar with
| @ajaychandran Overall this looks decent as a first pass, but what is missing is how this interacts with the existing  
 | 
| 
 @ajaychandran Let's focus on getting this PR merged first before we start looking at follow ups, I think there's enough work here to keep you occupied for at least a few more days, and I don't want us to get distracted and end up leaving work half finished | 
| import java.io.PrintStream | ||
| import scala.collection.immutable.SortedSet | ||
|  | ||
| trait PackageWriter { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should re-use as much of BuildGenUtil as possible here, which already has logic for rendering def mvnDeps, def moduleDeps, etc. We should not need to re-implement it from scratch and end up maintaining two different implementations. We should only need to implement that parts that BuildGenutil does not already implement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping to port the other importers to the new design. This might eliminate BuildGenUtil entirely.
Can we discuss this?
| val platform = crossVersion match { | ||
| case v: librarymanagement.Full if v.prefix.nonEmpty => "::" | ||
| case v: librarymanagement.Binary if v.prefix.nonEmpty => "::" | ||
| case v: librarymanagement.For2_13Use3 if v.prefix.nonEmpty => "_3" + "::" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just "_3::"? Same for the "_2.13" + "::" below
| The PR title and description have been updated. | 
| @ajaychandran Thanks for updating the PR description. Given this is a cleanroom implementation that aims to completely replace the old one, the PR description will need to be a lot more detailed to explain what the differences between the old implementation and new one are, to justify why it needs to be replaced and why the new implementation is better. As mentioned over email, this will need to be integrated into the existing utils and Maven/Gradle implementation before merging. While it's fine for you to experiment with a new design in a separate module, it needs to be fully integrated into the existing code before merging, so we can see the results of your new "better" architecture in a realistic scenarios with the same requirements. The old SBT implementation should be fully removed, and whatever code that can be shared between the new SBT implementation and the Maven/Gradle implementations should be consolidated | 
| 
 Added return type annotation for public methods. | 
| Diagnosis of a problem reported in a test case revealed an issue in Mill. For testing, the  | 
| 
 This is an issue specific to jcommander / TestNG. I saw it multiple times. IMHO, there is no appropriate generic fix for that in tooling. You can either workaround it (by reordering the classpath, if you're in the lucky case that jcommander versions are binary compatible) or properly fix it by changing the design of TestNG. Both approaches are out of scope for a generic build converter. | 
| 
 I didn't mean this is an issue with respect to conversion; this is an issue in Mill. | 
| 
 If unrelated, please open a new issue. | 
| @lihaoyi Is there anything pending here? | 
| @@ -0,0 +1,39 @@ | |||
| /* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all these files was generated by the Gradle 'init' task, can we generate them as part of our libs/init/package.mill in a generated resource folder? That would let us avoid committing all this generated code into main and having it potentiall drift or fall out of sync over time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not possible since the output of Gradle init command varies across versions and is not guaranteed to be backwards compatible.
The test projects were "minified" by removing comments and unused source files. The current line count is at 245.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, the unit tests can be revised to run the conversion on real world projects. We could target older releases to ensure Gradle version compatibility.
| val initRes = eval("init" +: initArgs) | ||
| assert(initRes.isSuccess) | ||
|  | ||
| if (configsGoldenFile != null) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's replace this and the one below with a single val taskNames: Seq[String] that we loop over and then .mkString into a single big string. That would remove all the duplication that comes from assigning things to local vals then interpolating them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, how is this different from the version in IntegrationTesterUtil, and the version below in renderAllImportedConfigurations? Can they be combined somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, rather than generating a flat file, we can do a single ./mill show __.{repositories,jvmId,mvnDeps,...} and which should return a JSON object we can parse and re-render with indent = 4, sortKeys = true as one big JSON object to use as our golden file contents. That will make it much easier for anyone reading these golden files later to understand what all these values mean, and avoid the messiness of the line-separated JSON objects you are using now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored test method to remove duplication.
Removed unused IntegrationTesterUtil and renderAllImportedConfigurations.
| @ajaychandran i left some last comments, let's give a day or so to see if @lefou has anything else, after that we can merge it | 
| @ajaychandran i think we can move forward. Could you resolve the conflicts one last time and then I'll merge it | 
# Conflicts: # libs/init/buildgen/src/mill/main/buildgen/BuildGenUtil.scala # libs/init/gradle/src/mill/main/gradle/GradleBuildGenMain.scala # libs/init/maven/src/mill/main/maven/MavenBuildGenMain.scala # libs/init/sbt/src/mill/main/sbt/SbtBuildGenMain.scala
| @ajaychandran seeing some failures in the  | 
| Retriggered the job, lets see how it goes | 
| @ajaychandran still seeing failures, does your branch include the latest  | 
| Yes, the  As per the log, the Mill server process fails to start.  | 
| Below is the error for a different test.  | 
| Let me try a more aggressive restart of all jobs rather than just the one that failed | 
This PR rewrites importer implementations using a new architecture that is easy to maintain and extend.
Motivation
While working on adding support for cross projects, the build models used in the existing implementations turned out to be insufficient for the use-case.
This prompted work on an alternate architecture that culminated in this pull request.
Solution
New data models are introduced to define a build specification. This includes an ADT whose members have a one-to-one correspondence with module types defined within Mill. Importer implementations were updated to target the new models.
New and dropped features
ErrorProneModulefor Gradle and Maven projects.jvmId.resources.Other improvements
PomSettingsin Maven conversion.artifactNamein SBT conversion.testsuite-osgimodule in netty project.ModelBuildingException.Limitations
sbt-crossprojectplugin.Summary of code changes
The previous implementation has been completely removed. Source files for application entry points and some tests were retained, but the contents have changed.
buildgen.apimodule. Together with new code inbuildgenthese account for ~1300 LOC.gradle.exportpluginmodule. The Gradle importer implementation and unit tests account for ~430 and ~700 LOC, respectively.Testing
integration.manual[migrating]module to develop and test the conversions against real world projects.Summary of fixes/enhancements made to improve support for test projects.
initerror in path handling forspring-ai.init/configuration error by removing custom resources forbyte-buddy/joda-beans.spring-boot-dependenciesBOM dependency formicroconfig.ErrorPluginModuleconfigurations formockito.Airstream,cats,enumeratum,fs2,nscala-time,refined,scala-logging,scopt,scrypto.Future work
sbt-projectmatrixplugin.